ImageDecoder+LibGfx: collate decoded bitmaps before sending over IPC #1435
Conversation
|
Hello! One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the |
|
Doesn't seem so? At least I was still experiencing the issue in #425 locally (as in the image decoder process would die when loading gifs with many frames). |
|
Also, even with the changes in #688, the issue seems to be that the image decoder is trying to send the type |
There was a problem hiding this comment.
Thanks for looking into this! It is indeed a very real problem that we can't handle large gifs, and create too many files.
I left some comments on the implementation. I think it could be more modular and factored. But overall the approach is sound, to create a metadata header followed by the actual data.
I'm not sure if it makes the most sense to structure it as
[ sequence of metadata ]
[ big blob ]
rather than
[ sequence of
[ metadata ]
[ blob for this image ]
]
But the result is the same, as long as we take care to sanity check all the size values before reading from the binary blobs.
|
Thanks for the feedback!
As far as I can tell, it necessarily has to be the first way, since each blob is encoded as a separate file (with a separate file descriptor). That's why I made it so that the separate blobs get merged into a single blob in order to send over IPC. Also, the existing code is already doing it the second way, which is where the problem stems from. |
There is an issue where gifs with many frames cannot be loaded, as each bitmap is sent over IPC using a separate file descriptor, and there is limit on the maximum number of descriptors per IPC message. Thus, trying to load gifs with more than 64 frames (the current limit) causes the image decoder process to die. This commit introduces the BitmapSequence class, which is a thin wrapper around the type Vector<Optional<NonnullRefPtr<Gfx::Bitmap>>> and provides an IPC encode/decode routine that collates all bitmap data into a single buffer so that only a single file descriptor is required per IPC transfer, even if multiple frames are being sent.
There was a problem hiding this comment.
This is great, thanks! I confirmed that the patch lets us load the gif from #425 as well. There's a few minor nits that I'll update in a follow-up. For future PRs, consider creating a branch per change set so that you aren't locked in to only have one PR open from your master branch at once :).
The Gfx::Bitmap encoder and decoder have been replaced by BitmapSequence in LadybirdBrowser#1435, making them redundant and safe to remove. Additionally, the base IPC::encode and IPC::decode implementations will trigger a compiler error if these methods are called. .ipc files have already been updated to exclude references to Gfx::Bitmap.
The Gfx::Bitmap encoder and decoder have been replaced by BitmapSequence in LadybirdBrowser#1435, making them redundant and safe to remove. Additionally, the base IPC::encode and IPC::decode implementations will trigger a compiler error if these methods are called. .ipc files have already been updated to exclude references to Gfx::Bitmap.
The Gfx::Bitmap encoder and decoder have been replaced by BitmapSequence in LadybirdBrowser#1435, making them redundant and safe to remove. Additionally, the base IPC::encode and IPC::decode implementations will trigger a compiler error if these methods are called. .ipc files have already been updated to exclude references to Gfx::Bitmap.
The Gfx::Bitmap encoder and decoder have been replaced by BitmapSequence in LadybirdBrowser#1435, making them redundant and safe to remove. Additionally, the base IPC::encode and IPC::decode implementations will trigger a compiler error if these methods are called. .ipc files already exclude references to Gfx::Bitmap.
The Gfx::Bitmap encoder and decoder have been replaced by BitmapSequence in #1435, making them redundant and safe to remove. Additionally, the base IPC::encode and IPC::decode implementations will trigger a compiler error if these methods are called. .ipc files already exclude references to Gfx::Bitmap.
There is an issue (#425) where gifs with many frames cannot be loaded, as each bitmap is sent over IPC using a separate file descriptor, and there is limit on the maximum number of descriptors per IPC message. Thus, trying to load gifs with more than 64 frames (the current limit) causes the image decoder process to die.
This commit introduces the
BitmapSequence class, which is a thin wrapper around the typeVector<Optional<NonnullRefPtr<Gfx::Bitmap>>>and provides an IPC encode/decode routine that collates all bitmap data into a single buffer so that only a single file descriptor is required per IPC transfer, even if multiple frames are being sent. This seems to fix the issue with large gifs (at least on my machine).Right now, I'm using
memcpyand pointer math to move data between AnonymousBuffers, which is probably not great for memory safety. If there's a better way of doing this that's more in line with project standards, please let me know.